📖 plan openshell integration#47
Conversation
Signed-off-by: David Zager <david.j.zager@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe documentation shifts execution from LLMProvider-based selection to OpenShell Gateway routing. It adds an ADR for gateway API sandbox creation, updates Agent and AgentRun relationships, revises infrastructure responsibilities, and documents gateway deployment and configuration. ChangesOpenShell execution model
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/adr/0004-openshell-as-execution-interface.md (1)
5-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMark the OpenShell migration claims as target state until implementation lands.
The documentation consistently describes the post-migration architecture, while the supplied controller still uses the legacy LLMProvider and direct Sandbox paths.
docs/adr/0004-openshell-as-execution-interface.md#L5-L11: label the replacement/removal statements as planned target state and link the migration prerequisite.CONTEXT.md#L109-L126: clarify that direct Agent Sandbox removal is not yet implemented.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/adr/0004-openshell-as-execution-interface.md` around lines 5 - 11, The OpenShell migration documentation presents planned architecture as implemented. In docs/adr/0004-openshell-as-execution-interface.md lines 5-11, label the replacement and LLMProvider removal statements as target state and link the migration prerequisite; in CONTEXT.md lines 109-126, clarify that direct Agent Sandbox removal is not yet implemented.CONTEXT.md (1)
128-136: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDefine the gateway TLS credential contract. The glossary and ADR say the controller connects with the gateway’s client TLS credentials, but they don’t say which Secret owns them, how they’re referenced, or how rotation is handled. Since an AgentRun only selects a gateway, this should be defined at the gateway level.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CONTEXT.md` around lines 128 - 136, Update the OpenShell Gateway definition in CONTEXT.md to specify the gateway-level Secret that owns the client TLS credentials, how the Gateway references that Secret, and the expected credential rotation behavior. Keep the contract scoped to each Gateway rather than AgentRun, and align the glossary wording with the corresponding ADR.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CONTEXT.md`:
- Around line 58-67: Update the AgentPlaybookRun contract documentation to
remove model/provider selections and define gateway references compatible with
AgentRun, including how the selected gateway is validated and used;
alternatively document a deterministic model-to-OpenShell-Gateway translation
that produces a valid AgentRun.
In `@docs/adr/0004-openshell-as-execution-interface.md`:
- Around line 45-55: Define the OpenShell Gateway discovery contract in the ADR:
specify the concrete Service naming scheme, required annotations, namespace
rules, readiness criteria, and controller resolution flow. Update the existing
“Agent references OpenShell Gateways” and “No Gateway CRD” sections so Hub and
the controller use the same discovery and validation behavior.
---
Nitpick comments:
In `@CONTEXT.md`:
- Around line 128-136: Update the OpenShell Gateway definition in CONTEXT.md to
specify the gateway-level Secret that owns the client TLS credentials, how the
Gateway references that Secret, and the expected credential rotation behavior.
Keep the contract scoped to each Gateway rather than AgentRun, and align the
glossary wording with the corresponding ADR.
In `@docs/adr/0004-openshell-as-execution-interface.md`:
- Around line 5-11: The OpenShell migration documentation presents planned
architecture as implemented. In
docs/adr/0004-openshell-as-execution-interface.md lines 5-11, label the
replacement and LLMProvider removal statements as target state and link the
migration prerequisite; in CONTEXT.md lines 109-126, clarify that direct Agent
Sandbox removal is not yet implemented.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6831ee94-fbd6-43f3-8a53-06f0ca7da548
📒 Files selected for processing (2)
CONTEXT.mddocs/adr/0004-openshell-as-execution-interface.md
| our behalf and injects its supervisor, which handles credential | ||
| isolation, inference routing, and policy enforcement. | ||
|
|
||
| **LLMProvider is removed.** The CRD, the controller, and all credential |
There was a problem hiding this comment.
Explain to me a bit more about the UX impact on this; is intention that we still allow users to define these things on our end and we do the heavy lifting to define the gateway CRs properly? Or is user expected to configure this separately?
There was a problem hiding this comment.
Looks like the answer to your question is it's all configured separately — gateways, providers/routes, and TLS creds live outside our CRDs (Helm + OpenShell CLI). That's a pretty different UX from LLMProvider being a CR you could GitOps. How are you thinking about the admin side — how they see what gateways exist, how it shows up in the UI?
There was a problem hiding this comment.
You're right that it's a different UX — imperative CLI instead of declarative CRs. The trade-off is intentional: we stop owning inference routing and credential management entirely, which is a lot of surface area we don't want to maintain.
For the admin side: Hub discovers gateways by listing Services in the namespace and exposes them through its REST API. The UI shows available gateways when creating Agents and AgentRuns. The admin manages gateways via Helm + OpenShell CLI, but users never touch OpenShell directly.
On GitOps: the Helm releases themselves are GitOps-able (values files in a git repo, ArgoCD/Flux deploys them). The provider/inference config is imperative today, but that's OpenShell's problem to solve — their Kubernetes operator issue (#1719) is where declarative config would land. It's a less great user experience today, but it's the right choice for us to align with OpenShell now and contribute upstream to improve it.
fabianvf
left a comment
fabianvf
left a comment
There was a problem hiding this comment.
Not for this PR, but AGENTS.md still calls Agent Sandbox the hard dependency and lists LLMProvider — probably worth a follow-up so it doesn't contradict this ADR.
| params (APP_ID, HUB_BASE_URL, etc.) for migration use cases. | ||
|
|
||
| **AgentPlaybookRun** — A request to execute an AgentPlaybook. References an | ||
| AgentPlaybook (or inlines the spec) and carries generic parameters, |
There was a problem hiding this comment.
This still says model selections, but AgentRun takes a gateway now — how does a stage's selection turn into a gateway? Don't think this can build a valid AgentRun as-is.
There was a problem hiding this comment.
Fixed in the latest push — AgentPlaybookRun now carries "a gateway selection" instead of "model selections." The playbook run selects one gateway and each stage's AgentRun inherits it.
| does not select a specific model — it declares what is available. | ||
| Gateway selection happens at execution time in the AgentRun. The | ||
| Agent controller validates that referenced gateways exist as Services | ||
| in the namespace and that SkillCards/SkillCollections are ready. |
There was a problem hiding this comment.
Agent doesn't compute context budget anymore, but the SkillCard entry at line 10 still talks about rules counting toward it. Is that just stale now, or does something still check it?
There was a problem hiding this comment.
Good catch — fixed in the latest push. Removed the "counts toward the LLM's context budget" clause from the SkillCard definition. Context budget validation was aspirational (the Agent controller doesn't implement it today either).
| # OpenShell as Execution Interface | ||
|
|
||
| Status: accepted | Date: 2026-07-16 |
There was a problem hiding this comment.
Formatting's a little different from the other ADRs — they use # ADR 0004: ..., the bold Status/Date/Authors block, and "Alternatives Considered". Also should this have a Supersedes: ADR 0001 note? It reverses the LLMProvider + direct Sandbox calls, and AGENTS.md mentions superseding rather than editing the old one.
There was a problem hiding this comment.
Fixed — aligned the heading, metadata block, and section names with the existing ADRs in the latest push.
On superseding: this doesn't supersede ADR-0001. ADR-0001 covers the full CRD architecture (seven CRDs, skill mounting, git-as-persistence, etc.) — most of which is unchanged. This ADR narrows the execution interface and removes LLMProvider. It's a refinement of one aspect, not a replacement of the whole decision.
| **Agent references OpenShell Gateways, not LLMProviders.** Each | ||
| OpenShell Gateway is a Kubernetes Service deployed via Helm, configured | ||
| with exactly one provider/model combination. An Agent declares one or | ||
| more gateways (the set available for runs). An AgentRun selects one |
There was a problem hiding this comment.
Looks like a gateway is locked to one provider+model in OpenShell (inference.local only points at one backend), so one-model-per-gateway is just what we get. How does that square with AgentRun taking multiple models with roles today (primary/efficient, we build KONVEYOR_MODEL_<ROLE>_* for each)? If a run needs two at once, does the AgentRun reference multiple gateways, or are we dropping multi-model per run?
There was a problem hiding this comment.
Dropping multi-model per run. inference.local gives you exactly one model per sandbox, so multi-role selection (primary/efficient) can't work through OpenShell's routing. An AgentRun selects one gateway = one model.
The multi-role design was speculative — not exercised in any tests or real usage beyond validation. Added an explicit section to ADR-0004 documenting this: "Multi-model per run is dropped."
If a future need arises for multi-model within a single run, it would require either OpenShell adding per-sandbox inference routing (not on their roadmap) or the agent calling external endpoints directly with per-sandbox provider credentials (possible but loses the privacy router benefits).
| - The `sigs.k8s.io/agent-sandbox` Go module dependency is replaced by | ||
| the OpenShell Go SDK. The SDK is in-progress (NVIDIA/OpenShell#2270) | ||
| and must land before this migration can be implemented. |
There was a problem hiding this comment.
Running a bunch of gateways in a shared namespace and picking one per run is basically OpenShell's multi-tenant/operator story, which still looks open upstream (NVIDIA/OpenShell#1719, #1722, #1795) — seems like it's single-user / one gateway per deployment today. So this probably depends on multi-gateway landing along with the Go SDK (#2270). Does that make it more target-state than Accepted?
There was a problem hiding this comment.
Good references, but our design doesn't depend on those upstream issues. We're not doing multi-tenancy — we're running multiple independent gateway Helm releases in the same namespace. Each has its own StatefulSet, Service, database, and TLS Secrets. They don't share state. This is just helm install with different release names — it works today.
The upstream issues are about different problems:
- #1719 — should OpenShell have a Kubernetes operator/CRDs (we use the gateway API directly)
- #1722 — serving multiple tenants from a shared gateway (we use separate gateways)
- #1795 — creating sandboxes in different namespaces (we're single-namespace)
The only upstream blocker is the Go SDK (#2270), already called out in Consequences. "Accepted" means the decision is made — implementation dependencies don't change the status.
Updated the Deployment Model section to make this distinction explicit and added the upstream issue references. We're not primarily concerned with how those gateways get installed — we can contribute to OpenShell to make that part easier.
Expand ADR-0004 with a Deployment Model section covering OpenShell's namespace-scoped gateway model, the requirement that all gateways live in the shared namespace, UX impact per persona, and multi-tenancy limitations. Update CONTEXT.md OpenShell Gateway entry to clarify the namespace constraint. Signed-off-by: David Zager <david.j.zager@gmail.com>
Clarify in ADR-0004 that running multiple independent gateway Helm releases in one namespace is not multi-tenancy and does not depend on upstream OpenShell work (NVIDIA/OpenShell#1719, #1722, #1795). Add upstream issue references to the multi-tenancy limitations section. Update AgentPlaybookRun in CONTEXT.md to reference gateway selection instead of stale model selections. Signed-off-by: David Zager <david.j.zager@gmail.com>
djzager
left a comment
djzager
left a comment
There was a problem hiding this comment.
Responding to review comments inline — see individual thread replies below.
Updated the ADR Deployment Model section to clarify that multi-gateway != multi-tenancy, linked the upstream OpenShell issues (#1719, #1722, #1795), and fixed the stale AgentPlaybookRun reference in CONTEXT.md.
- Align ADR-0004 heading and metadata with ADR-0001 format - Rename 'Considered Options' to 'Alternatives Considered' - Document dropping multi-model per run (inference.local = one model) - Remove stale context budget reference from SkillCard in CONTEXT.md Signed-off-by: David Zager <david.j.zager@gmail.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
CONTEXT.md (2)
110-127: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftCorrect the Agent Sandbox ownership statement.
The current
AgentRunReconcilerhas Sandbox CRUD permissions and its documented flow creates a Sandbox CR directly, which contradicts “does not interact with Agent Sandbox directly” and “sandboxes are created through the OpenShell gateway API.” Mark this as target architecture or update the reconciler, RBAC, and documentation together.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CONTEXT.md` around lines 110 - 127, Align the documented architecture with the current implementation: either label the OpenShell-mediated Agent Sandbox ownership statement as target architecture, or update AgentRunReconciler to create sandboxes through the OpenShell gateway API, remove its direct Sandbox CRUD permissions, and revise the related flow documentation consistently.
31-41: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSynchronize the glossary with the current CRD contracts.
The supplied APIs still expose
AgentSpec.Providers,AgentRunSpec.Models, andAgentPlaybookRunSpec.Models/PlaybookRef; they do not define the gateway fields described here. The current reconciler also validates model selections rather than gateway membership. As written, this documents a future API and can lead clients to create invalid manifests. Either update the API/controller in the same change or explicitly mark these sections as target-state documentation with a migration plan.Also applies to: 58-65, 69-73, 163-166
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CONTEXT.md` around lines 31 - 41, Update the glossary sections for Agent, AgentRun, and AgentPlaybookRun to match the current CRD fields and reconciler behavior, including Providers, Models, and PlaybookRef instead of undocumented gateway concepts. Do not describe gateway selection or gateway validation as current behavior; if retaining that model, explicitly label it target-state documentation and include a migration plan.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@CONTEXT.md`:
- Around line 110-127: Align the documented architecture with the current
implementation: either label the OpenShell-mediated Agent Sandbox ownership
statement as target architecture, or update AgentRunReconciler to create
sandboxes through the OpenShell gateway API, remove its direct Sandbox CRUD
permissions, and revise the related flow documentation consistently.
- Around line 31-41: Update the glossary sections for Agent, AgentRun, and
AgentPlaybookRun to match the current CRD fields and reconciler behavior,
including Providers, Models, and PlaybookRef instead of undocumented gateway
concepts. Do not describe gateway selection or gateway validation as current
behavior; if retaining that model, explicitly label it target-state
documentation and include a migration plan.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e741b7d-f16e-4059-837b-63febfadbcb4
📒 Files selected for processing (2)
CONTEXT.mddocs/adr/0004-openshell-as-execution-interface.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/adr/0004-openshell-as-execution-interface.md
Add 'Current state', 'What changes', and 'Prerequisites' sections to the ADR Context so readers understand what exists today vs what changes when the Go SDK lands. Call out the RBAC change (agents.x-k8s.io permissions removed) in Consequences. Signed-off-by: David Zager <david.j.zager@gmail.com>
3 participants
Summary by CodeRabbit